Skip to content

test: cancel upload by observing progress, not wall-clock sleep (flaky test) #6421

Closed
FarhanAliRaza wants to merge 2 commits intoreflex-dev:mainfrom
FarhanAliRaza:flaky-test
Closed

test: cancel upload by observing progress, not wall-clock sleep (flaky test) #6421
FarhanAliRaza wants to merge 2 commits intoreflex-dev:mainfrom
FarhanAliRaza:flaky-test

Conversation

@FarhanAliRaza
Copy link
Copy Markdown
Contributor

A fixed asyncio.sleep before clicking cancel races the browser's upload process: under CI load the test can be preempted long enough for the throttled upload to finish, after which abort() can no longer prevent the file from being written. Poll the progress display for an in-flight event (0 < progress < 1) and cancel only then.

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

A fixed asyncio.sleep before clicking cancel races the browser's upload process: under CI load the test can be preempted long enough for the throttled upload to finish, after which abort() can no longer prevent the file from being written. Poll the progress display for an in-flight event (0 < progress < 1) and cancel only then.
@FarhanAliRaza FarhanAliRaza requested a review from a team as a code owner April 29, 2026 14:56
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 29, 2026

Merging this PR will not alter performance

✅ 9 untouched benchmarks


Comparing FarhanAliRaza:flaky-test (497de71) with main (8f7ad9a)

Open in CodSpeed

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 29, 2026

Greptile Summary

This PR fixes a flaky test by replacing asyncio.sleep with a DOM-polling helper (expect_upload_in_flight) that waits until the upload is verifiably in-flight (0 < progress < 1) before clicking cancel, tying cancel timing to actual upload state rather than wall-clock time. The final assertions are also tightened to only examine progress entries created during the current test run via an initial_progress_count slice.

Confidence Score: 4/5

Safe to merge; test-only change with a well-designed fix for a known flaky pattern.

Only one P2 style observation about diagnostics on timeout; the core logic is sound and the approach directly addresses the documented race condition.

No files require special attention beyond the one P2 note on expect_upload_in_flight.

Important Files Changed

Filename Overview
tests/integration/test_upload.py Replaces racy asyncio.sleep with a DOM-polling helper that waits for an in-flight upload progress event before cancelling; also scopes post-cancel assertions to only the current run's progress entries via initial_progress_count slicing.

Sequence Diagram

sequenceDiagram
    participant Test as Test (pytest)
    participant Browser as Browser / WebDriver
    participant DOM as DOM (progress_dicts)
    participant Server as Upload Server

    Test->>Browser: send_keys(target_file)
    Test->>Browser: upload_button.click()
    Browser->>Server: Start throttled upload (1 Mbps)
    loop poll every 250ms (≤15s)
        Test->>DOM: find_elements(progress_xpath)[initial_count:]
        DOM-->>Test: progress entries
        Test->>Test: any 0 < progress < 1?
    end
    Note over Test: in-flight confirmed
    Test->>Browser: cancel_button.click()
    Browser->>Server: abort()
    Test->>Test: asyncio.sleep(12) — upload would finish if cancel failed
    Test->>DOM: find_elements(progress_xpath)[initial_count:]
    DOM-->>Test: progress entries
    Test->>Test: assert no progress == 1
    Test->>Server: assert file not written
Loading

Reviews (1): Last reviewed commit: "test: cancel upload by observing progres..." | Re-trigger Greptile

Comment thread tests/integration/test_upload.py
@masenf
Copy link
Copy Markdown
Collaborator

masenf commented Apr 29, 2026

supersede by #6424 with various improvements

@masenf masenf closed this Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants